London|May 2025|Alexandru Pocovnicu|Coursework/sprint 3 #639
Conversation
alexandru-pocovnicu
added
the
Needs Review
alexandru-pocovnicu
changed the title
cjyuan
added
Review in progress
Sprint-3/3-mandatory-practice/implement/get-ordinal-number.test.js
Outdated
Show resolved
Hide resolved
| test("should repeat the string count times", () => { | ||
| const str = "hello"; | ||
| const count = -1; | ||
| const repeatedStr = repeat(str, count); | ||
| expect(repeatedStr).toEqual("negative number"); | ||
| }); |
There was a problem hiding this comment.
If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)
There was a problem hiding this comment.
thank you but I couldn't understand the documetation
cjyuan
added
Reviewed
|
Hi, I don't know what's happening, all the files in sprint 3 from my computer are empty, as if i haven't done any work in them , is there a way I could copy the ones on github, should i clone the whole PR and then push that , will i have to create a new PR? |
|
never mind the previous message , sorted....i was in the wrong branch |
alexandru-pocovnicu
added
Needs Review
| if (Math.abs(numerator) < Math.abs(denominator)) { | ||
| return true}else{ | ||
| return false; |
There was a problem hiding this comment.
Can you improve the indentation of this code?
This is the suggested style by CYF: https://curriculum.codeyourfuture.io/guides/reviewing/style-guide/
| const allrank=["2","3","4","5","6","7","8","9","10"] | ||
| if(allrank.includes(rank))return Number(rank); | ||
| if(rank==="10"|| rank==="J" || rank==="Q" || rank==="K") return 10; | ||
| else return "Invalid card rank" | ||
| else return "Invalid card rank"; |
There was a problem hiding this comment.
You have taken care of "10" at line 6, so you don't have to check for rank==="10" at line 7.
|
|
||
| test("should return invalid cards for anything else", () => { | ||
| expect(getCardValue("2.1♠")).toBe("Invalid card rank"); | ||
| }); | ||
| test("should return invalid cards for anything else", () => { | ||
| expect(getCardValue("00_02♠")).toBe("Invalid card rank"); | ||
| }); |
There was a problem hiding this comment.
Could combine these into one tests, as both of them are testing for invalid card ranks.
| @@ -1,8 +1,12 @@ | |||
| function repeat(str,count) { | |||
| if(count<0){ | |||
| return "negative number" | |||
| return false; | |||
There was a problem hiding this comment.
I strongly suggest looking up "How to throw error in JavaScript".
| @@ -22,27 +22,32 @@ test("password has at least 5 characters", () => { | |||
| const result = isValidPassword(password); | |||
| // Assert | |||
| expect(result).toEqual(true); | |||
| expect(isValidPassword("12h")).toEqual(false) | |||
There was a problem hiding this comment.
An string that contains all required characters except that it is too short would make a better test value because when the function return false, we would know exactly why it returns false.
cjyuan
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.